-
-
Notifications
You must be signed in to change notification settings - Fork 0
Support string.Format culture info parameter analyzer & code fixer #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThis pull request makes minor formatting adjustments in the localization analyzers. In the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Localization.Analyzers/Localize/OldGetTranslateAnalyzer.cs (1)
43-60
: Iterating over arguments to detect nested translations.This expanded check allows detecting any translation calls within
string.Format
arguments. Nicely handles reporting a diagnostic when found. Consider whether multiple translations in the samestring.Format
are a valid scenario that might require more than a single diagnostic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Localization.Analyzers/Localize/OldGetTranslateAnalyzer.cs
(4 hunks)Flow.Launcher.Localization.Analyzers/Localize/OldGetTranslateAnalyzerCodeFixProvider.cs
(5 hunks)Flow.Launcher.Localization.Analyzers/Localize/OldGetTranslateAnalyzerCodeFixProvider.cs
(3 hunks)Flow.Launcher.Localization.Analyzers/Localize/OldGetTranslateAnalyzerCodeFixProvider.cs
(1 hunks)Flow.Launcher.Localization.Analyzers/Localize/OldGetTranslateAnalyzer.cs
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (20)
Flow.Launcher.Localization.Analyzers/Localize/OldGetTranslateAnalyzerCodeFixProvider.cs (6)
52-53
: Gracefully handling null root.It's good to see a concise check for a null
root
to prevent downstream exceptions. If possible, confirm whether you expectroot
to be non-null in most scenarios. If it's always expected to be non-null, consider logging a debug message or clarifying why this condition might be triggered, so that potential anomalies can be caught earlier.
58-58
: Safer retrieval with FirstOrDefault.Switching from
First()
toFirstOrDefault()
prevents potentialInvalidOperationException
if no matching node is found. This aligns well with the subsequent null checks.
60-60
: Early return when invocation expression is null.Returning the original document when
invocationExpr
is null is a clean fallback. This helps avoid unnecessary processing down the line.
64-83
: Iterating over all arguments.Looping through each argument to handle both literal and nested invocation scenarios is robust. Note that the logic currently stops after handling the first matching argument. If future requirements call for detecting multiple translations in the same call, consider extending the loop to handle them all.
113-113
: Comparing count with == 1 instead of using a pattern.Using
== 1
for checking the argument count is more typical and explicit than pattern matching for older compilers. This improves clarity.
126-130
: New parameter and logic for string formatting.Adding the
int translationArgIndex
parameter clarifies which argument contains the translation call. Skipping arguments up totranslationArgIndex + 1
is a neat approach. However, if multiple translation calls exist, only the first is handled. Consider whether a multi-invocation scenario should be supported.Flow.Launcher.Localization.Analyzers/Localize/OldGetTranslateAnalyzer.cs (14)
14-14
: Region annotation.No functional changes here; purely organizational. Nothing to address.
27-27
: Region end.Again, purely organizational. No concerns.
29-29
: Region annotation.No functional changes here; purely organizational. Nothing to address.
37-37
: Clarifying comment for format string call.This comment clarifies the logic. No concerns.
40-40
: Additional comment explaining first branch detection.Clear explanation of matching calls to
string.Format
. No issues flagged.
62-62
: Comment for second branch.No functional changes, purely descriptive.
79-80
: Enhanced argument retrieval with an index.Adding an overload to retrieve additional arguments beyond the translation call is helpful. This is a good step toward more dynamic handling of arguments.
82-101
: Implementing IsParentFormatStringCall to walk up the syntax tree.Walking up the tree to check for an enclosing format call is a clean approach. Implementation seems robust, and short-circuiting when you find a matching symbol is efficient.
104-105
: Check for string.Format method.Your conditions for recognizing
string.Format
are concise and correct. No issues here.
107-108
: Retrieving nested argument invocation.Using a helper method to fetch nested invocation expressions by index is consistent with the rest of your design. This improves readability.
112-112
: Using OldLocalizationMethodName for translation calls.Straightforward string check is aligned with the approach used elsewhere. No remarks.
125-125
: Region boundary annotation.Purely organizational. No functional changes.
126-126
: Final region boundary.Again, organizational. Nothing to address.
128-128
: Concluding region.No functional changes. Skipping details.
Important
I have thoroughly tested the generator diagnostics and code fixes related to the old translation and I believe them are ready to move to production.
Support string.Format culture info parameter analyzer & code fixer
Test
Now fixing them can work well.